assemble-repository: Prevent sources from being included inadvertently #3635

sratz · 2024-03-12T17:12:22Z

In the presence of products, P2 adds a virtual tooling.source.default
IU which optionally depends on all sources.

If these sources are available in the target platform, this would cause
them to be picked up even if assemble-repository was configured with

  <includeAllDependencies>true<includeAllDependencies>

but with

  <includeAllSources>false</includeAllSources>.

This scenario has become much more likely with recent changes to P2 [1].

We do not want includeAllDependencies to imply includeAllSources, so
we actively prevent this from happening by ignoring the

  <required namespace='org.eclipse.equinox.p2.eclipse.type'
            name='source'
            range='0.0.0'
            optional='true'
            multiple='true'
            greedy='false'/>

requirements if includeAllSources is not specified.

Fixes #3522.

[1] eclipse-equinox/p2#446

github-actions · 2024-03-12T18:56:52Z

Test Results

591 files +3 591 suites +3 4h 39m 20s ⏱️ + 2m 35s
407 tests +2 400 ✅ +4 7 💤 ±0 0 ❌ - 1
1 221 runs +6 1 199 ✅ +9 22 💤 ±0 0 ❌ - 1

Results for commit a028cbf. ± Comparison against base commit d6366ad.

♻️ This comment has been updated with latest results.

laeubi

It goes in the right direction but basically I would

check here if this is a requirement for a source bundle
Construct a context IU with org.eclipse.update.install.sources=true/false
then check if they match in isApplicable(IInstallableUnit iu, IRequirement req)

that way the requirements will just be filtered out.

laeubi · 2024-03-14T07:47:15Z

tycho-core/src/main/java/org/eclipse/tycho/p2tools/TychoMirrorApplication.java

@@ -118,6 +119,10 @@ protected Slicer createSlicer(SlicingOptions options) throws ProvisionException
        boolean considerOnlyStrictDependency = options.considerStrictDependencyOnly();
        return new PermissiveSlicer(repository, filters.get(0), includeOptionalDependencies,
                options.isEverythingGreedy(), evalFilterTo, considerOnlyStrictDependency, onlyFilteredRequirements) {
+
+            private static final String TOOLING_SOURCE_IU_ID = PublisherHelper.createDefaultConfigUnitId("source",


I'm not sure this is the correct approach also the test failuresindicate that.

laeubi · 2024-03-14T07:51:09Z

tycho-core/src/main/java/org/eclipse/tycho/p2tools/TychoMirrorApplication.java

+
+            private static final String TOOLING_SOURCE_IU_ID = PublisherHelper.createDefaultConfigUnitId("source",
+                    "tooling");
+
            @Override
            protected boolean isApplicable(IInstallableUnit iu, IRequirement req) {
                if ((includeRequiredBundles || includeRequiredFeatures) && QueryUtil.isGroup(iu)


I think this is actually the place where you want to filter things, this test if a requirement for a IU is applicable and you don't want the new requirement to be considered.

sratz · 2024-03-15T14:10:42Z

1. check here if this is a requirement for a source bundle
2. Construct a context IU with org.eclipse.update.install.sources=true/false

I'm not sure I understand this part.
Where is 'here'?
What's a 'context IU'?
Also, as the problem is not the org.eclipse.update.install.sources, but the fact that a published product contains a tooling.source.default IU which optionally depends on all sources.

3. then check if they match in isApplicable(IInstallableUnit iu, IRequirement req)

I think this is the important aspect. We don't want to filter
tooling.source.default itself in isApplicable(IInstallableUnit iu)
but only filter
tooling.source.default's dependencies in isApplicable(IInstallableUnit iuIRequirement req).

I have moved the condition to isApplicable(IInstallableUnit iuIRequirement req), let's see what the test suite says.

laeubi · 2024-03-15T14:17:00Z

I must confess I'm not sure tooling.source.default is really the problem. This has not changed recently but what has changed is that each bundle now has the org.eclipse.update.install.sources requirement.

sratz · 2024-03-15T14:20:14Z

I must confess I'm not sure tooling.source.default is really the problem. This has not changed recently but what has changed is that each bundle now has the org.eclipse.update.install.sources requirement.

It has not changed, but before org.eclipse.update.install.sources was introduced those sources weren't part of the target platform.

Now they are, and the effect of tooling.source.default now pulls them in.

sratz · 2024-03-15T14:23:19Z

An alternative to the current proposal of looking at tooling.source.default could be to provide a filtered view on the project's target platform with org.eclipse.update.install.source=true IUs excluded. Then, using that filtered target platform to initiate the mirroring. But I am not sure I know how to do that.

laeubi · 2024-03-18T07:56:03Z

As said this are not IUs but Requirements ... You can create a so called "context IU" from a map of properties with InstallableUnit.contextIU(context) in your case maybe InstallableUnit.contextIU(Map.of(org.eclipse.update.install.source, true)), if you have that you can use

IMatchExpression<IInstallableUnit> filter = req.getFilter();
booelan matches =  filter != null && filter.isMatch(contextIU);

for example to check if a requirement is matching that context IU and if yes return false from is applicable if includeAllSources is false.

sratz · 2024-03-18T08:48:49Z

As said this are not IUs but Requirements ... You can create a so called "context IU" from a map of properties with InstallableUnit.contextIU(context) in your case maybe InstallableUnit.contextIU(Map.of(org.eclipse.update.install.source, true)), if you have that you can use
IMatchExpression<IInstallableUnit> filter = req.getFilter();
booelan matches =  filter != null && filter.isMatch(contextIU);
for example to check if a requirement is matching that context IU and if yes return false from is applicable if includeAllSources is false.

I tried this:

            private static final IInstallableUnit SOURCEC_CONTEXT = InstallableUnit
                    .contextIU(Map.of("org.eclipse.update.install.source", "true"));

            @Override
            protected boolean isApplicable(IInstallableUnit iu, IRequirement req) {
                if (!includeAllSource) {
                    IMatchExpression<IInstallableUnit> filter = req.getFilter();
                    if (filter != null) {
                        if (filter.isMatch(SOURCEC_CONTEXT)) {
                            // When dealing with a repository that contains published products, these products
                            // always include a dependency to 'tooling.source.default', which picks up all sources.
                            // And since https://github.com/eclipse-equinox/p2/pull/446 the target platform contains
                            // all the sources. They would be picked up unless we prevent this here.
                            // See https://github.com/eclipse-tycho/tycho/issues/3522
                            return false;
                        }
                    }
                }

but it does not work.

As outlined in the issue, the problematic IU is tooling.source.default which has an IRequirement on all source providers:

		// Create a required capability on source providers
		IRequirement[] reqs = new IRequirement[] {MetadataFactory.createRequirement(NAMESPACE_ECLIPSE_TYPE, TYPE_ECLIPSE_SOURCE, VersionRange.emptyRange, null, true, true, false)};
		cu.setHost(reqs);
		Map<String, String> touchpointData = new HashMap<>();

https://github.com/eclipse-equinox/p2/blob/e0ce87e039a001c4aafcbb2dba6ca33455933097/bundles/org.eclipse.equinox.p2.publisher/src/org/eclipse/equinox/spi/p2/publisher/PublisherHelper.java#L169-L189

This requirement does not have any filter so the code above will not be able to filter anything here based on the context IU.

laeubi · 2024-03-18T09:19:48Z

@merks yna you give a hint about the tooling.source.default IU?

merks · 2024-03-18T09:53:04Z

One thing that might generally be helpful here is that this is an optional non-greedy requirement:

<unit id='tooling.source.default' version='1.0.0' singleton='false'>
  <hostRequirements size='1'>
    <required namespace='org.eclipse.equinox.p2.eclipse.type' name='source' range='0.0.0' optional='true' multiple='true' greedy='false'/>
  </hostRequirements>
  <properties size='1'>
    <property name='org.eclipse.equinox.p2.type.fragment' value='true'/>
  </properties>
  <provides size='2'>
    <provided namespace='org.eclipse.equinox.p2.iu' name='tooling.source.default' version='1.0.0'/>
    <provided namespace='org.eclipse.equinox.p2.flavor' name='tooling' version='1.0.0'/>
  </provides>
  <requires size='1'>
    <required namespace='org.eclipse.equinox.p2.eclipse.type' name='source' range='0.0.0' optional='true' multiple='true' greedy='false'/>
  </requires>
...
</unit>

As far as I understand it, at installation time, an optional non-greedy requirement will not be installed even when it's available, i.e., there must be some other non-optional requirements (in which case the required things must be available), or optional greedy requirement (in which case it's installed when available and quietly ignored when not).

Of course the context here is mirroring and not installing. I think in the filtering context here that generally it should be possible to ignore all optional non-greedy requirements. I'm not sure if that should involve yet another configuration option...

laeubi · 2024-03-18T09:55:09Z

As far as I know "inlcudeAllRequirements" makes everything greedy....

sratz · 2024-03-18T10:44:11Z

Yes, we have includeAllDependencies and includeAllSources.

I think the intuitive expectation is that includeAllDependencies does not also imply includeAllSources.

And previously this expectation was (usually) met, because the sources typically weren't visible via the target platform in these kind of product-containing repository definitions.

So the current proposal of this PR basically ensures that this expectation is met deterministically.

sratz · 2024-03-18T11:05:04Z

Instead of looking at the IU's name tooling.source.default, we could also just look at the IRequirement and do this:

                if (!includeAllSource && req.getMin() == 0 && !req.isGreedy()
                        && req instanceof IRequiredCapability capability
                        && PublisherHelper.NAMESPACE_ECLIPSE_TYPE.equals(capability.getNamespace())
                        && PublisherHelper.TYPE_ECLIPSE_SOURCE.equals(capability.getName())) {
                    return false;
                }

WDYT?

merks · 2024-03-18T11:29:48Z

That's exactly what I was trying just now:

I was just making sure the tests still pass but was having problems with the 4.31 repository not loading for some unknown reason.

merks · 2024-03-18T11:30:54Z

@sratz I suggest you make the change (yes, the test still passes) since I'm a bit incompetent at updating someone else's PR.

sratz · 2024-03-18T11:31:27Z

I was just making sure the tests still pass but was having problems with the 4.31 repository not loading for some unknown reason.

had the same, clearing~.m2/cache helped.

merks · 2024-03-18T11:34:38Z

@sratz

FYI, this test fails for me locally as well:

So I'm looking at that...

In the presence of products, P2 adds a virtual 'tooling.source.default' IU which optionally depends on all sources. If these sources are available in the target platform, this would cause them to be picked up even if assemble-repository was configured with <includeAllDependencies>true<includeAllDependencies> but with <includeAllSources>false</includeAllSources>. This scenario has become much more likely with recent changes to P2 [1]. We do not want includeAllDependencies to imply includeAllSources, so we actively prevent this from happening by ignoring the <required namespace='org.eclipse.equinox.p2.eclipse.type' name='source' range='0.0.0' optional='true' multiple='true' greedy='false'/> requirements if 'includeAllSources' is not specified. Fixes eclipse-tycho#3522. [1] eclipse-equinox/p2#446

merks · 2024-03-18T11:52:38Z

@sratz

The test failure is unrelated and is already fixed in master:

tycho/tycho-core/src/test/java/org/eclipse/tycho/p2resolver/P2ResolverTest.java

Lines 225 to 226 in d6366ad

    
           assertEquals(4, result.getArtifacts().size()); 
        
           assertEquals(4, result.getNonReactorUnits().size()); // + a.jre

The problem is that the test uses a moving target repository so the actual content can and apparently does change:

tycho/tycho-core/src/test/java/org/eclipse/tycho/p2resolver/P2ResolverTest.java

Line 75 in d6366ad

    
           private static final String LATEST_PLATFORM = "https://download.eclipse.org/eclipse/updates/latest/";

Please ensure that you rebase your change on master.

outdated

laeubi

Looks good and is backed by test so I think its ready to go.

eclipse-tycho-bot · 2024-03-18T14:09:13Z

💚 All backports created successfully

Status	Branch	Result
✅	tycho-4.0.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation and see the Github Action logs for details

merks · 2024-03-18T14:12:57Z

And a back port too. That's great! Thanks!!

laeubi · 2024-03-18T14:17:57Z

@sratz many thanks for the fix, as you already have contributed to Tycho for a while I'd like to ask if you want to become a committer I just would need your email address of the eclipse account and can prepare the vote then!

sratz · 2024-03-20T09:45:13Z

@sratz many thanks for the fix, as you already have contributed to Tycho for a while I'd like to ask if you want to become a committer I just would need your email address of the eclipse account and can prepare the vote then!

Thanks for the offer! I pinged you via email.

laeubi previously requested changes Mar 14, 2024

View reviewed changes

sratz force-pushed the product-sources branch from 380bcac to bfffc6e Compare March 15, 2024 14:06

sratz force-pushed the product-sources branch from bfffc6e to a028cbf Compare March 18, 2024 11:40

laeubi added the backport-to-tycho-4.0.x Can be added to a PR to trigger an automatic backport of the change label Mar 18, 2024

laeubi approved these changes Mar 18, 2024

View reviewed changes

laeubi merged commit 51e39d6 into eclipse-tycho:main Mar 18, 2024
12 checks passed

eclipse-tycho-bot mentioned this pull request Mar 18, 2024

[tycho-4.0.x] assemble-repository: Prevent sources from being included inadvertently #3679

Merged

sratz deleted the product-sources branch March 20, 2024 09:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

assemble-repository: Prevent sources from being included inadvertently #3635

assemble-repository: Prevent sources from being included inadvertently #3635

sratz commented Mar 12, 2024 •

edited

Loading

github-actions bot commented Mar 12, 2024 •

edited

Loading

laeubi left a comment

laeubi Mar 14, 2024

laeubi Mar 14, 2024

sratz commented Mar 15, 2024 •

edited

Loading

laeubi commented Mar 15, 2024

sratz commented Mar 15, 2024

sratz commented Mar 15, 2024

laeubi commented Mar 18, 2024 •

edited

Loading

sratz commented Mar 18, 2024

laeubi commented Mar 18, 2024

merks commented Mar 18, 2024

laeubi commented Mar 18, 2024

sratz commented Mar 18, 2024

sratz commented Mar 18, 2024 •

edited

Loading

merks commented Mar 18, 2024

merks commented Mar 18, 2024

sratz commented Mar 18, 2024

merks commented Mar 18, 2024

merks commented Mar 18, 2024

laeubi left a comment

eclipse-tycho-bot commented Mar 18, 2024

merks commented Mar 18, 2024

laeubi commented Mar 18, 2024

sratz commented Mar 20, 2024

assemble-repository: Prevent sources from being included inadvertently #3635

assemble-repository: Prevent sources from being included inadvertently #3635

Conversation

sratz commented Mar 12, 2024 • edited Loading

github-actions bot commented Mar 12, 2024 • edited Loading

Test Results

laeubi left a comment

Choose a reason for hiding this comment

laeubi Mar 14, 2024

Choose a reason for hiding this comment

laeubi Mar 14, 2024

Choose a reason for hiding this comment

sratz commented Mar 15, 2024 • edited Loading

laeubi commented Mar 15, 2024

sratz commented Mar 15, 2024

sratz commented Mar 15, 2024

laeubi commented Mar 18, 2024 • edited Loading

sratz commented Mar 18, 2024

laeubi commented Mar 18, 2024

merks commented Mar 18, 2024

laeubi commented Mar 18, 2024

sratz commented Mar 18, 2024

sratz commented Mar 18, 2024 • edited Loading

merks commented Mar 18, 2024

merks commented Mar 18, 2024

sratz commented Mar 18, 2024

merks commented Mar 18, 2024

merks commented Mar 18, 2024

laeubi left a comment

Choose a reason for hiding this comment

eclipse-tycho-bot commented Mar 18, 2024

💚 All backports created successfully

Questions ?

merks commented Mar 18, 2024

laeubi commented Mar 18, 2024

sratz commented Mar 20, 2024

sratz commented Mar 12, 2024 •

edited

Loading

github-actions bot commented Mar 12, 2024 •

edited

Loading

sratz commented Mar 15, 2024 •

edited

Loading

laeubi commented Mar 18, 2024 •

edited

Loading

sratz commented Mar 18, 2024 •

edited

Loading